-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
testscript: add RequireUniqueNames parameter #185
Conversation
7370561
to
69978e7
Compare
69978e7
to
0c21eae
Compare
The test failure seems unrelated to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Apologies it took so long to get back to you. There's also a conflict, but I imagine it's a trivial one.
[linux] testscript -unique-names scripts-case-insensitive | ||
|
||
[windows] ! testscript -unique-names scripts-case-insensitive | ||
[windows] stdout 'duplicate name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do Mac and Windows always use case insensitive filesystems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We both know that they don't always :) It's rare, but possible, for macOS users to run their systems with case sensitive filesystems, for example, even though this tends to break a large number of macOS applications.
The effort of making the tests detect whether the filesystem they're running on is case sensitive or not is fairly large. I think you'd have to create a file with a long random lowercase name (to avoid conflicts with any existing files) and test if it exists with the name uppercased. You'd have to be careful to use the exact directory that the tests run in. You can't use a temporary directory as it by be mounted as a different filesystem.
So, for now, I've removed these parts of the tests.
0c21eae
to
d0a44e3
Compare
The conflict is indeed trivial, but the tests no longer pass. Specifically, the test that testscript should return an error when duplicate names are encountered fails. I've traced this as far as verifying that I suspect that #192 might be related. Specifically, does #192 cope with |
Please rebase and try again :) |
d0a44e3
to
ba5f698
Compare
Thanks both! I've rebased and now the test passes :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one suggestion, thanks!
testscript/testscript.go
Outdated
if ts.params.RequireUniqueNames { | ||
switch _, err := os.Lstat(name); { | ||
case err == nil: | ||
ts.Fatalf("'%s' would overwrite '%s' (because RequireUniqueNames is enabled)", f.Name, name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use %q rather than single quotes.
But how about an alternative approach? Instead of using an explicit LStat
, just pass the write options to os.OpenFile
such that it will refuse to overwrite an existing file.
You could define an alternative write-file function to make this easy; for example:
func writeFile(name string, data []byte, excl bool) error {
oflags := os.O_WRONLY | os.O_CREATE | os.O_TRUNC
if excl {
oflags |= os.O_EXCL
}
f, err := os.OpenFile(name, oflags, 0o666)
if err != nil {
return err
}
defer f.Close()
if _, err := f.Write(data); err != nil {
return fmt.Errorf("cannot write file contents: %v", err)
}
return nil
}
Then below, do:
ts.Check(writeFile(name, f.Data, ts.params.RequireUniqueNames)
I think the resulting "file exists" error is probably sufficient, although a check of the error with os.IsExist(err)
could be used to customise it.
This way we're doing exactly the same amount of work as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how about an alternative approach? Instead of using an explicit
LStat
, just pass the write options toos.OpenFile
such that it will refuse to overwrite an existing file.
Yes, this is a much better approach.
This way we're doing exactly the same amount of work as before.
It's also more resistant to race conditions: creating an exclusive file is a single atomic syscall, whereas lstat/open is vulnerable to races if the file is created between the lstat and the open.
Thanks for the feedback. I'll update this PR with your suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point Roger, I missed this!
9f8b9b5
to
b6415ea
Compare
Co-authored-by: Roger Peppe <[email protected]>
b6415ea
to
6799a50
Compare
Fixes #184.